Only put Debug-like bounds on type variables #371
Conversation
sornas
force-pushed
the
debug-bounds-only-participating-type-variables
branch
2 times, most recently
from
776409d
to
f13c7d9
Compare
sornas
force-pushed
the
debug-bounds-only-participating-type-variables
branch
from
f13c7d9
to
16713df
Compare
impl/src/fmt/debug.rs
Outdated
| path.segments.iter().any(|segment| { | ||
| self.type_variables | ||
| .iter() | ||
| .any(|type_var| *type_var == &segment.ident) |
There was a problem hiding this comment.
This is outside my Rust knowledge, but I'm starting to think this is wrong. If the type is a path (like std::vec::Vec), then it cannot contain a type variable, correct? We only care if it is the only segment or itself a type variable (T in std::vec::Vec<T>) at some part of the path.
There was a problem hiding this comment.
Yes, correct. I think an easy way to test that would be:
#[derive(derive_more::Debug)]
struct Vec {
next: std::vec::Vec<i32>,
}
impl/src/fmt/debug.rs
Outdated
| syn::Type::Infer(_) => false, | ||
| syn::Type::Macro(_) => false, | ||
| syn::Type::Never(_) => false, | ||
| syn::Type::TraitObject(_) => false, |
There was a problem hiding this comment.
Trait objects can contain type parameters:
trait MyTrait<T> {}
struct Test<T>
{
t: Box<dyn MyTrait<T>>,
}
|
@sornas I have some time to work on derive_more the next ~2 weeks. Since this is one of the last items that's blocking 1.0, I was wondering if you'll have time to work on this during that time. Otherwise I think I might pick it up to get it over the finish line, if I'm able to get the other 1.0 blockers done. |
|
unfortunately i don't think i'll have time, so feel free to take over. but i'll let you know here if it changes. |
JelteF
force-pushed
the
debug-bounds-only-participating-type-variables
branch
from
6968c49
to
659dfdb
Compare
JelteF
force-pushed
the
debug-bounds-only-participating-type-variables
branch
from
659dfdb
to
6131444
Compare
Resolves #363 Requires #377 Follows #371 ## Synopsis The problem is that the `Display` derive adds bounds for all types that are used in the format string. But this is not necessary for types that don't contain a type variable. And adding those bounds can result in errors like for the following code: ```rust #[derive(Display, Debug)] #[display("{inner:?}")] #[display(bounds(T: Display))] struct OptionalBox<T> { inner: Option<Box<T>>, } #[derive(Display, Debug)] #[display("{next}")] struct ItemStruct { next: OptionalBox<ItemStruct>, } ``` That code would generate the following error: ```text error[E0275]: overflow evaluating the requirement `ItemStruct: derive_more::Display` ``` ## Solution This makes sure we don't add unnecessary bounds for Display-like derives. It does so in the same way as #371 did for the Debug derive: By only adding bounds when the type contains a type variable. Co-authored-by: Kai Ren <tyranron@gmail.com>
|
This change resulted in a regression, after updating from 1.0.0-beta.6 to 1.0.0-beta.7 compilation fails in deltachat/deltachat-core-rust#5850 |
|
Hmm, that's not great. Can you make a new issue for this that shows some code that now fails compilation? |
|
Update: seems to be fixed (or worked around?) in n0-computer/iroh#2578 |
|
I opened an issue #392 |
Resolves #392 Related to #371 ## Synopsis Before #371 this code would compile without any issues, but that would actually hide the problem that the resulting `Debug` implementation was never applicable, because the bounds could not be satisfied. ## Solution This problem was already solved by #371, but this adds a test case to ensure that we don't regress here again.
Resolves #363
Well, at least it's a suggestion for a resolution :p
Synopsis
The problem, as reported in the issue, is that code like the following
expands into something like
which does not compile. This PR changes the Debug derive so it does not emit those bounds.
Solution
My understanding of the current code is that we iterate over all fields of the struct/enum and add either a specific
format bound (e.g.
: fmt::Binary), a default: fmt::Debugbound or skip it if either it is markedas
#[debug(skip)]or the entire container has a format attribute.The suggested solution in the issue (if I understood it correctly) was to only add bounds if the type is a type
variable, since rustc already knows if a concrete type is, say,
: fmt::Debug. So, instead of adding the bound forevery type, we first check that the type contains one of the container's type variables. Since types can be nested, it
is an unfortunately long recursive function handling the different types of types. This part of Rust syntax is probably
not going to change, so perhaps it is feasible to shorten some of the branches into
_ => false.One drawback of this implementation is that we iterate over the list of type variables every time we find a "leaf type".
I chose
VecoverHashSetbecause in my experience there are only a handful of type variables per container.Status
I should add more tests, probably? I've at least verified the case mentioned above (for tuple structs, data structs and
enums). I must admit I haven't really looked in the documentation, but I should probably edit that too.
Checklist
Documentation is updated(not required)CHANGELOG entry is added(not required)